Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build: remove dependency on Python sass module #34439

Merged
merged 1 commit into from
Apr 5, 2024
Merged

build: remove dependency on Python sass module #34439

merged 1 commit into from
Apr 5, 2024
+96 −28

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Mar 28, 2024 •
edited
Loading

Supporting information

Depends on:

Part of:

Description

Instead of using libsass-python's sass module, which is incompatible
with Python 3.11, we now skip through to libsass-python's _sass module,
which directly binds to the underlying C++ libsass library.

This ensures that the Sass build will not a blocker for the edx-platform
Python 3.11 upgrade, which needs to land in Redwood.

For more details, see the docstring at scripts/compile_sass.py#L167

Testing

Same as #34318 (comment)

Merge considerations

None

@kdmccormick kdmccormick mentioned this pull request Mar 28, 2024
Merged
This was referenced Mar 28, 2024
Closed
Open
@kdmccormick kdmccormick self-assigned this Mar 28, 2024
@kdmccormick kdmccormick mentioned this pull request Mar 28, 2024
Closed
@kdmccormick kdmccormick force-pushed the kdmccormick/libsass-python-2 branch 2 times, most recently from ff01a70 to f390c02 Compare April 3, 2024 17:09
@kdmccormick kdmccormick marked this pull request as ready for review April 3, 2024 17:09
@feanil feanil self-requested a review April 3, 2024 19:12
feanil
feanil approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I was able to build the assets and load the site on Node 16.10

@kdmccormick
Copy link
Member Author

Thanks @feanil , do I have your thumbs up on the prerequisite PR too? #34318

@kdmccormick kdmccormick added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Apr 4, 2024
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@kdmccormick
build: remove dependency on Python sass module
3c829fa 
Instead of using libsass-python's `sass` module, which is incompatible
with Python 3.11, we now directly use libsass-python's `_sass` module,
which directly binds to the underlying C++ libsass library.

For more details, see the docstring at scripts/compile_sass.py#L167
@kdmccormick kdmccormick merged commit a08a10c into openedx:master Apr 5, 2024
67 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/libsass-python-2 branch April 5, 2024 14:50
@kdmccormick kdmccormick mentioned this pull request Apr 5, 2024
Merged
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@dianakhuang
Copy link
Contributor

Stuck in a meeting so can't verify that this is the source, but it seems likely, but we're seeing this error in our builds:

Traceback (most recent call last):
  File "scripts/compile_sass.py", line 505, in <module>
    main(prog_name="npm run compile-sass --")
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "scripts/compile_sass.py", line 411, in main
    compile_sass_dir(
  File "scripts/compile_sass.py", line 262, in compile_sass_dir
    for sass_path in glob.glob(str(source) + "/**/*.scss"):
UnboundLocalError: local variable 'source' referenced before assignment

dianakhuang pushed a commit that referenced this pull request Apr 5, 2024
@kdmccormick @dianakhuang
Revert "build: remove dependency on Python sass module (#34439)"
2bea4b5 
This reverts commit a08a10c.
@kdmccormick
Copy link
Member Author

@dianakhuang Oof, seems likely, feel free to merge #34476

kdmccormick
kdmccormick commented Apr 5, 2024
View reviewed changes
scripts/compile_sass.py
with open(target, 'w', encoding="utf-8") as target_file:
target_file.write(output_text)

click.secho(f" Done.", fg="green")
# For Sass files without explicit RTL versions, generate
# an RTL version of the CSS using the rtlcss library.
for sass_path in glob.glob(str(source) + "/**/*.scss"):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, the issue is on this line, I should have changed source to source_root. This made it through my local testing because source is a local variable in the new loop above this one, which was erroneously being used here, leading a silent failure.

I imagine that 2U must have a theme without any matched scss files, meaning that the local source variable is never assigned, leading a to a loud failure instead in their pipelines instead of a quiet one.

kdmccormick added a commit that referenced this pull request Apr 5, 2024
@kdmccormick
revert: "build: remove dependency on Python sass module (#34439)" (#…
a27cda2 
…34476)

This reverts commit a08a10c.

compile_sass.py had a bug which was caught by 2U's pipeline, but not
by local testing.
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@kdmccormick kdmccormick mentioned this pull request Apr 11, 2024
Merged
KyryloKireiev pushed a commit to raccoongang/edx-platform that referenced this pull request Apr 24, 2024
@kdmccormick @KyryloKireiev
build: remove dependency on Python sass module (openedx#34439)
171d29f 
Instead of using libsass-python's `sass` module, which is incompatible
with Python 3.11, we now directly use libsass-python's `_sass` module,
which directly binds to the underlying C++ libsass library.

This ensures that the Sass build will not a blocker for the edx-platform
Python 3.11 upgrade, which needs to land in Redwood.

For more details, see the docstring at scripts/compile_sass.py#L167
KyryloKireiev pushed a commit to raccoongang/edx-platform that referenced this pull request Apr 24, 2024
@kdmccormick @KyryloKireiev
revert: "build: remove dependency on Python sass module (openedx#34439
860ad12 
)" (openedx#34476)

This reverts commit a08a10c.

compile_sass.py had a bug which was caught by 2U's pipeline, but not
by local testing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feanil feanil feanil approved these changes

Assignees

@kdmccormick kdmccormick

Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kdmccormick @open-craft-grove @dianakhuang @edx-pipeline-bot @feanil